test: run bun handwritten tests in bun runtime for firestore through pubsub plus spanner-driver - #9452
Conversation
Adds bin/run-test.cjs and bin/proxyquire-bun-shim.cjs to run Mocha tests across both Node.js and Bun without breaking Node coverage or parallelism. When invoked under Node.js, bin/run-test.cjs delegates to c8 and Mocha with worker-thread parallelism enabled. When invoked under Bun (via bun --bun or JS_RUNTIME=bun), it skips c8, disables Mocha worker threads (--no-parallel), preloads the Bun proxyquire compatibility shim, and executes Mocha directly in-process so #!/usr/bin/env node shebangs do not silently switch execution back to Node.js.
…gnal timeout override
…ogging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage
…into bun-runtime/1-test-runner-handwritten-libraries # Conflicts: # core/packages/gax/.mocharc.js
This reverts commit 5a29104.
This reverts commit c9446ab.
…ps://github.com/googleapis/google-cloud-node into bun-runtime/1-test-runner-handwritten-libraries
This reverts commit 4c4a74d.
…ime" This reverts commit 486a301.
…bun runtime"" This reverts commit 687e386.
This reverts commit 756cc45.
…into bun-runtime/1-test-runner-handwritten-libraries-2
…ttps://github.com/googleapis/google-cloud-node into bun-runtime/1-test-runner-handwritten-libraries-2
…th bun tests instead
…gging, logging-bunyan, logging-winston, pubsub, spanner, spanner-driver, and storage" This reverts commit 8088cf4.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request standardizes test execution across multiple packages by replacing direct mocha and c8 invocations with a centralized test runner script ('run-test.cjs'). It also addresses a deadlock issue in the Pub/Sub tests under Bun by separating the queue addition and flushing operations, and disables automatic diagnostic instrumentation during logging tests. The reviewer suggested simplifying the Pub/Sub test fix by sequentially awaiting the flush and completion promises instead of using Promise.all.
| const completion = modAckQueue.add(new FakeMessage() as Message, 10); | ||
| await Promise.all([completion, modAckQueue.flush('test')]); |
There was a problem hiding this comment.
Instead of using Promise.all to concurrently await the completion promise and the flush operation, you can sequentially await them. Since add() synchronously queues the message and returns a promise, calling flush() immediately after will trigger the flush and resolve the completion promise. Awaiting them sequentially is simpler, avoids the complexity of Promise.all, and prevents potential unhandled promise rejections if one of them fails.
| const completion = modAckQueue.add(new FakeMessage() as Message, 10); | |
| await Promise.all([completion, modAckQueue.flush('test')]); | |
| const completion = modAckQueue.add(new FakeMessage() as Message, 10); | |
| await modAckQueue.flush('test'); | |
| await completion; |
There was a problem hiding this comment.
This is the opposite of what was suggested 2 suggestions ago. I think we can stick with the code as it is.
bshaffer
left a comment
There was a problem hiding this comment.
Can we simplify node ../../bin/run-test.cjs --config ../../.mocharc.cjs build/test to just run-test build/test?
- If we expose
run-testinbinon a workspace package (e.g.gapic-toolsor a package incore/dev-packages) and add it to the rootpackage.jsondevDependencies,pnpmwill link it into the rootnode_modules/.binso it is onPATHfor all workspace packages. - We can drop
--config ../../.mocharc.cjs— Mocha already walks up directories to find the root.mocharc.cjswhen no local config exists, and passing--config ../../.mocharc.cjshere overrides the package-local.mocharc.jsconfigs inhandwritten/*(likefirestore/.mocharc.js).
This reverts commit 55e9e98.
Yeah. I looked into this and the windows tests seem to have trouble with .. which can be addressed by prepending with node, but I did what you said and added a link to tools for all workspace packages. |
Actually, I suggest we do #2, but not #1. Adding a link to the gapic-tools directory doesn't work because it makes the system tests fail. GCB does not include the gapic-tools directory unless you add it to devDependencies which isn't worth it for a minor simplification. |
…evert gapic-tools changes
Agreed, SGTM |
Description
Updates handwritten packages across the repository to use the runtime-agnostic test runner (
bin/run-test.cjs) and addresses test compatibility issues when running under the Bun runtime:run-test.cjs: Updatedtestandsystem-testscripts inpackage.jsonforbigquery,bigquery-storage,datastore,error-reporting,firestore,google-cloud-dns,logging,logging-bunyan,logging-winston,pubsub, andspanner-driverto usenode ../../bin/run-test.cjs --config ../../.mocharc.cjs.handwritten/datastore:@grpc/grpc-jsand@grpc/proto-loadertodevDependenciesand updatedpnpm-lock.yamlto ensure mock server tests resolve gRPC dependencies under pnpm's strict package isolation."null is not an object (evaluating 'data.name.toString')") alongside existing V8/Node patterns intest/index.ts.handwritten/bigquery-storage:test/adapt/proto.tssodeepStrictEqualassertions and Prettier formatting pass cleanly across runtimes.handwritten/error-reporting:configuration.ts,service-configuration.ts) to avoid cross-test pollution during non-parallel execution.handwritten/logging:instrumentation.setInstrumentationStatus(true)intest/log-sync.tsbeforeEachto prevent automatic diagnostic log emission from polluting file stream output during sequential test runs.handwritten/pubsub:test/message-queues.ts(ModAckQueue->should send call options) by capturing the completion promise and flushing the queue before awaiting completion.Impact
bun --bun run test).Scope
Libraries included: firestore, google-cloud-dns, logging, logging-bunyan, logging-winston, pubsub
Remaining: bigtable, spanner, storage
We also updated the handwritten libraries that are already done so that they use the test-runner with a simpler script without updating gapic tools.